Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

another one wide char file API recover #3117

Merged
merged 4 commits into from
Jan 19, 2025
Merged

another one wide char file API recover #3117

merged 4 commits into from
Jan 19, 2025

Conversation

w17
Copy link
Contributor

@w17 w17 commented Jan 13, 2025

It doesn't cover all cases.
Just provides ability to open L"unicode.exif" files.

src/basicio.cpp Outdated Show resolved Hide resolved
@neheb
Copy link
Collaborator

neheb commented Jan 14, 2025

I can't say I like C APIs here.

Would

path_ = stringFormat({:s}, wstr);

work here?

edit: nope.

@neheb neheb merged commit 43ab673 into Exiv2:main Jan 19, 2025
3 checks passed
@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

I'd rather revert this PR to be honest and let it percolate a bit more.

I don't think all _WIN32 should use this code path, things work just fine w/ UCRT builds and UTF-8 code page on "modern" Windows.

Any restoration of the -W API (I personally wouldn't bother) should be done with greater care.

@kmilos kmilos mentioned this pull request Jan 20, 2025
@neheb
Copy link
Collaborator

neheb commented Jan 20, 2025

Right.

OTOH, actual users of the library are being impacted. It doesn't seem to be practical to force people to move to UCRT.

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

It doesn't seem to be practical to force people to move to UCRT.

UCRT is available for 97% of Windows users, it is the default target for Visual Studio since version 2017 I think, and is available for GNU and LLVM toolchains since cca 2019.

It's been almost 2 years since this happened (not saying it should've when it did, it was perhaps a bit premature). But today: Windows 7 EOL'd 5 years ago, Windows 8.1 2 years ago. I really think it's not worth putting the milk back into the bottle. If some passionate legacy users still really need it, it should be done very carefully.

@neheb
Copy link
Collaborator

neheb commented Jan 20, 2025

I don't think it's practical to tell users to use toolchain Y to compile exiv2, when toolchain X works perfectly fine for everything else.

If I look on Fedora for example, the UCRT MinGW toolchain is there but lacks many packages. It's even worse on Ubuntu 24.04, effectively telling users MSVCRT and UCRT are no go for their applications.

@kmilos
Copy link
Collaborator

kmilos commented Jan 20, 2025

It's even worse on Ubuntu 24.04

Ubuntu does lag behind unfortunately. It is however now available in 24.10 (and Debian Trixie). Admittedly bare bones as Fedora so less convenient, but not a showstopper... Just out of interest, how many users do you know cross-compiling?

works perfectly fine

And let's not forget the legacy approach was not ideal either: the wmain input argument conversion wrapper is a security hole, and there is a non-trivial -W API code overhead to maintain.

@neheb
Copy link
Collaborator

neheb commented Jan 20, 2025

From the issue in Issues, at least digikam.

wmain is only for the exiv2 binary, no? The main issue here is with the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants